fix(mcp): clarify request wrapper in list_datasets, list_charts, list_dashboards#39920
Conversation
…ts, list_dashboards
LLMs consistently passed flat kwargs (search, page, page_size) to list_*
tools instead of wrapping them in the required `request` object, causing
pydantic validation errors.
- Add docstring usage examples to list_datasets, list_charts, and
list_dashboards showing the correct `request={...}` call shape and
explicitly warning against flat kwargs
- Enumerate valid filter columns directly in DatasetFilter, ChartFilter,
and DashboardFilter field descriptions (e.g. `created_by_fk` is not a
valid dataset filter col)
- Add TestListDatasetsRequestWrapper tests covering: correct request
wrapper usage, default values, valid/invalid filter col validation,
and the flat-kwargs rejection scenario from story #105712
- Allow E501 in list_*.py tool files (docstring examples need full request
shapes to be instructive)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39920 +/- ##
==========================================
- Coverage 63.88% 63.87% -0.01%
==========================================
Files 2583 2583
Lines 136604 136625 +21
Branches 31502 31504 +2
==========================================
Hits 87276 87276
- Misses 47812 47833 +21
Partials 1516 1516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR reduces recurring MCP tool invocation errors by making the required request={...} wrapper explicit in the list_datasets, list_charts, and list_dashboards tool docstrings, and by clarifying/validating allowed filter columns in the corresponding Pydantic schemas.
Changes:
- Added prominent docstring guidance + examples showing the required
request={...}call shape (and warning that flat kwargs are rejected). - Updated
DatasetFilter/ChartFilter/DashboardFilterfield descriptions to enumerate validcolvalues. - Added unit tests focused on the
list_datasetsrequest-wrapper/validation scenario, and added a Ruff per-file ignore forE501inlist_*.pytool modules.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py | Adds tests for request-wrapper expectations and invalid filter column validation. |
| superset/mcp_service/dataset/tool/list_datasets.py | Adds an IMPORTANT docstring block and example call shapes for request={...}. |
| superset/mcp_service/dataset/schemas.py | Clarifies allowed DatasetFilter.col values in schema field description. |
| superset/mcp_service/dashboard/tool/list_dashboards.py | Adds docstring guidance/examples for request-wrapped parameters. |
| superset/mcp_service/dashboard/schemas.py | Clarifies allowed DashboardFilter.col values in schema field description. |
| superset/mcp_service/chart/tool/list_charts.py | Adds docstring guidance/examples for request-wrapped parameters. |
| superset/mcp_service/chart/schemas.py | Clarifies allowed ChartFilter.col values in schema field description. |
| pyproject.toml | Adds Ruff per-file ignore for E501 in MCP list_*.py tool files to accommodate long docstring examples. |
- Fix test_sortable_columns_in_docstring: assertion used plain-text match
but docstring now uses RST backtick format; use substring check for both
'Sortable columns for' and 'order_column' separately
- Fix test_dataset_filter_valid_col: opr='ct' is not a valid
ColumnOperatorEnum value; use opr='sw' (starts-with) instead
- Add test_request_wrapper_enforced_by_tool: exercises the wrapper pattern
through the actual FastMCP client call (not just schema instantiation),
verifying the MCP tool layer accepts request={...} correctly
- Strengthen test_flat_kwargs_rejected: assert that the ToolError message
references the unexpected arguments ('search', 'Unexpected', or 'request')
so the test cannot pass on unrelated failures
…oard docstring test - Replace opr:"ct" with opr:"sw" in filter examples in list_datasets, list_charts, and list_dashboards docstrings — "ct" is not a valid ColumnOperatorEnum value, so examples using it produce validation errors - Fix TestDashboardSortableColumns.test_sortable_columns_in_docstring: assertion checked for "Sortable columns for order_column:" (plain text) but docstring uses RST backtick format; split into two substring checks matching "Sortable columns for" and "order_column" separately
Code Review Agent Run #0d41fdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
…port All 6 new test methods in TestListDatasetsRequestWrapper were missing the required -> None return type annotation (Rule 12). Also remove the inline `from fastmcp.exceptions import ToolError` in test_flat_kwargs_rejected since ToolError is already imported at module top-level (line 27).
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
LLMs consistently call
list_datasets(and related list tools) with flattop-level kwargs like
search,page,page_sizeinstead of wrapping themin the required
requestobject, resulting in pydantic validation errors:This PR makes the calling convention explicit in the tool docstrings and
improves filter column descriptions to enumerate valid values.
Changes:
list_datasets,list_charts, andlist_dashboardsshowing the correctrequest={...}call shape, withan explicit warning that flat kwargs are rejected
colvalues directly inDatasetFilter,ChartFilter,and
DashboardFilterfield descriptions (e.g.created_by_fkis not avalid dataset filter column)
TestListDatasetsRequestWrapperunit tests covering: correct wrapperusage, defaults, valid/invalid
colvalidation, and the flat-kwargsrejection scenario
E501inlist_*.pytool files in ruff config (docstring exampleswith full request shapes exceed the 88-char limit)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — docstring and schema description changes only.
TESTING INSTRUCTIONS
list_datasets,list_charts,list_dashboards— theIMPORTANTblock and example usage should beclearly visible.
ADDITIONAL INFORMATION